Introduce device profiles #129
Conversation
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: nojnhuh The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
k8s-ci-robot
added
cncf-cla: yes
|
I wanted to open this PR before I'm off on vacation next week, but I intend to take a look at #128 first and incorporate those changes here before merging this. FYI @guptaNswati /hold |
k8s-ci-robot
added
the
do-not-merge/hold
|
@sunya-ch If you could take a look to see if your consumable capacity changes are able to align with this that would be great! |
sunya-ch
left a comment
sunya-ch
left a comment
There was a problem hiding this comment.
@nojnhuh
Could we define a DeviceProfile interface/struct with abstract methods like GetDevices, ApplyConfig, and ValidateConfig, and then keep a mapping from profile name to its implementation?
If we take that approach, I could implement ConsumableNetDevProfile or ConsumableGPUProfile independently. What do you think?
k8s-ci-robot
added
size/XXL
internal/profiles/profiles.go
Outdated
|
|
||
| // Validate implements [ConfigHandler]. | ||
| func (n NoopConfigHandler) Validate(config runtime.Object) error { | ||
| return errors.New("no configuration allowed") |
There was a problem hiding this comment.
Is it expected behavior to return an error? The error message confuse me a bit whether it is allowed or error.
There was a problem hiding this comment.
Overall, the goal of the NoopConfigHandler is to disallow any configuration to prevent users from supplying configuration that the driver doesn't know how to handle. I can see how the message can be unclear though whether it's read as "no (configuration allowed)" or "(no configuration) allowed" so I've reworded this to "configuration not allowed". Returning an error here is intended to help enforce that. Does that help clarify?
internal/profiles/profiles.go
Outdated
|
|
||
| // ApplyConfig implements [ConfigHandler]. | ||
| func (n NoopConfigHandler) ApplyConfig(config runtime.Object, results []*resourceapi.DeviceRequestAllocationResult) (PerDeviceCDIContainerEdits, error) { | ||
| return nil, errors.New("no configuration allowed") |
There was a problem hiding this comment.
same as Validate. The wording confuse me a bit.
| } | ||
|
|
||
| // SchemeBuilder implements [profiles.ConfigHandler]. | ||
| func (p Profile) SchemeBuilder() runtime.SchemeBuilder { |
There was a problem hiding this comment.
It might be more straightforward to have ConfigHandler implemented separately from Profile.
There was a problem hiding this comment.
I think these are logically coupled enough that it makes sense to keep them together. Tying the profile and config handling together in the profile I think makes more sense then somewhere higher-level like main.go. Implementing them together makes initialization a bit simpler: "--device-profile => Profile (also implementing ConfigHandler)" vs. "--device-profile => separate Profile and ConfigHandler". I don't think we'd ever want a user to specify the profile and config handler independently.
|
@sunya-ch: changing LGTM is restricted to collaborators In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
3 participants
This PR lays the groundwork for the example driver to be able to demonstrate several different kinds of devices, each implemented as a separate "profile." The changes mostly involve extracting the GPU-specific details and exposing handles to plug in different logic.
To see how this works with multiple profiles, I have a POC based on these changes implementing a new
gpupartprofile which demonstrates partitionable devices: nojnhuh/dra-example-driver@profiles...gpupartFixes #92